Fix data grid undo/redo and multi-row deletion bugs#4
Conversation
datlechin
commented
Dec 23, 2025
datlechin
commented
- Fix undo insert row then save error by adding safety checks in generateSQL()
- Fix multi-row delete only deleting last row by implementing undoBatchRowInsertion()
- Fix redo after delete causing 'Could not generate SQL' error
- Fix Cmd+Z not restoring deleted inserted rows by adding batchRowInsertion undo action
- Add proper undo/redo support for batch operations
- Clean up verbose debug logging, keep only warnings
- Fix undo insert row then save error by adding safety checks in generateSQL() - Fix multi-row delete only deleting last row by implementing undoBatchRowInsertion() - Fix redo after delete causing 'Could not generate SQL' error - Fix Cmd+Z not restoring deleted inserted rows by adding batchRowInsertion undo action - Add proper undo/redo support for batch operations - Clean up verbose debug logging, keep only warnings
There was a problem hiding this comment.
Pull request overview
This PR fixes several critical bugs related to data grid undo/redo operations and multi-row deletion. The changes add proper batch operation support for deleting multiple inserted rows simultaneously, implement safety checks to prevent SQL generation errors, and clean up verbose debug logging.
Key changes:
- Added
batchRowInsertionundo action andundoBatchRowInsertion()method to handle deleting multiple inserted rows as a single atomic operation - Implemented safety checks in
generateSQL()to verify rows are still in the correct state before generating SQL statements - Refactored
deleteSelectedRows()to separate inserted vs existing row handling and properly manage batch deletions - Added
rebuildTableQuery()utility to ensure queries reflect current filter/sort state when refreshing
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| SidebarView.swift | Removed debug print statements for pendingDeletes operations |
| MainContentView.swift | Refactored multi-row deletion logic, added undo/redo handlers for batch row insertion, improved refresh flow, removed debug logging |
| DataChange.swift | Added batch row insertion undo action, implemented undoBatchRowInsertion(), added safety checks in generateSQL(), enhanced undo/redo logic |
| project.pbxproj | Bumped version from 0.1.3 to 0.1.4 and build number from 3 to 4 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Collect row values BEFORE removing changes (for undo/redo) | ||
| var rowValues: [[String?]] = [] | ||
| for rowIndex in validRows { | ||
| if let insertChange = changes.first(where: { $0.rowIndex == rowIndex && $0.type == .insert }) { | ||
| let values = insertChange.cellChanges.sorted { $0.columnIndex < $1.columnIndex } | ||
| .map { $0.newValue } | ||
| rowValues.append(values) | ||
| } else { | ||
| rowValues.append(Array(repeating: nil, count: columns.count)) | ||
| } | ||
| } | ||
|
|
||
| // Remove all INSERT changes for these rows | ||
| for rowIndex in validRows { | ||
| changes.removeAll { $0.rowIndex == rowIndex && $0.type == .insert } | ||
| insertedRowIndices.remove(rowIndex) | ||
| } | ||
|
|
||
| // Push undo action so user can undo this deletion | ||
| pushUndo(.batchRowInsertion(rowIndices: validRows, rowValues: rowValues)) |
There was a problem hiding this comment.
There's a potential index mismatch bug between rowIndices and rowValues arrays. The code collects rowValues by iterating through validRows (filtered from rowIndices), but stores the undo action with validRows as rowIndices. If some rows are filtered out as invalid, the arrays will have different lengths, causing the indices to be mismatched when accessing rowValues[index] in the undo handler at line 1581 in MainContentView.swift. The arrays should always be in sync with the same length and order, either by using validRows consistently or by collecting rowValues only for valid rows and storing them in the same order.
| // Process in reverse order (ascending) to maintain correct indices | ||
| for (index, rowIndex) in rowIndices.enumerated().reversed() { | ||
| guard index < rowValues.count else { continue } | ||
| guard rowIndex <= tabManager.tabs[tabIndex].resultRows.count else { continue } | ||
|
|
||
| let values = rowValues[index] |
There was a problem hiding this comment.
The undo logic for batchRowInsertion has an index mismatch. The code uses rowIndices.enumerated().reversed() which pairs each index with its position in the array (0, 1, 2...), but then uses that enumeration index to access rowValues[index]. This means if rowIndices is [5, 3, 1], the enumeration will be [(2, 5), (1, 3), (0, 1)] when reversed, and you'll access rowValues[2], rowValues[1], rowValues[0]. However, rowValues was built by iterating through validRows in forward order, so rowValues[0] corresponds to validRows[0], not necessarily rowIndices[0]. The correct approach is to iterate through the arrays together using zip or indices: for (rowIndex, values) in zip(rowIndices, rowValues).reversed()
| // Process in reverse order (ascending) to maintain correct indices | |
| for (index, rowIndex) in rowIndices.enumerated().reversed() { | |
| guard index < rowValues.count else { continue } | |
| guard rowIndex <= tabManager.tabs[tabIndex].resultRows.count else { continue } | |
| let values = rowValues[index] | |
| // Process in reverse order to maintain correct indices when inserting | |
| for (rowIndex, values) in zip(rowIndices, rowValues).reversed() { | |
| guard rowIndex <= tabManager.tabs[tabIndex].resultRows.count else { continue } | |
| let rowsDeleted = insertedRowsToDelete.count | ||
| let adjustedMaxRow = maxSelectedRow - rowsDeleted |
There was a problem hiding this comment.
The logic for calculating adjustedMaxRow is incorrect. It subtracts the total count of deleted inserted rows from maxSelectedRow, but it should only subtract the number of deleted inserted rows that are BELOW maxSelectedRow, not all of them. If you delete rows [1, 5, 7] and maxSelectedRow is 5, the adjusted position should be 5 - 1 = 4 (only row 1 is below it), not 5 - 3 = 2. The correct calculation should be: insertedRowsToDelete.filter { $0 < maxSelectedRow }.count
| let rowsDeleted = insertedRowsToDelete.count | |
| let adjustedMaxRow = maxSelectedRow - rowsDeleted | |
| let rowsDeletedBelowMax = insertedRowsToDelete.filter { $0 < maxSelectedRow }.count | |
| let adjustedMaxRow = maxSelectedRow - rowsDeletedBelowMax |
| // Redo the deletion of inserted rows - remove them again | ||
| // This is called when user: delete inserted rows -> undo -> redo | ||
| // We need to remove the rows from changes and insertedRowIndices again | ||
| for rowIndex in rowIndices { |
There was a problem hiding this comment.
The redo logic for batchRowInsertion doesn't handle index shifting. When removing multiple inserted rows from changes and insertedRowIndices, the code iterates through rowIndices without adjusting for the fact that removing rows changes the indices of subsequent rows. This should either process rowIndices in descending order to avoid index shifting issues, or should apply the same index shifting logic used in undoBatchRowInsertion lines 419-435. Without this, remaining inserted rows will have incorrect indices after the redo operation.
| for rowIndex in rowIndices { | |
| // Process in descending index order to avoid index-shifting issues | |
| let sortedRowIndices = rowIndices.sorted(by: >) | |
| for rowIndex in sortedRowIndices { |
| case .batchRowInsertion(let rowIndices, let rowValues): | ||
| // Undo the deletion of inserted rows - restore them as INSERT changes | ||
| // Process in reverse order (ascending) to maintain correct indices when re-inserting | ||
| for (index, rowIndex) in rowIndices.enumerated().reversed() { | ||
| guard index < rowValues.count else { continue } | ||
| let values = rowValues[index] | ||
|
|
||
| // Re-create INSERT change | ||
| let cellChanges = values.enumerated().map { colIndex, value in | ||
| CellChange( | ||
| rowIndex: rowIndex, | ||
| columnIndex: colIndex, | ||
| columnName: columns[safe: colIndex] ?? "", | ||
| oldValue: nil, | ||
| newValue: value | ||
| ) | ||
| } | ||
| let rowChange = RowChange(rowIndex: rowIndex, type: .insert, cellChanges: cellChanges) | ||
| changes.append(rowChange) | ||
| insertedRowIndices.insert(rowIndex) | ||
| } |
There was a problem hiding this comment.
The undo logic for batchRowInsertion doesn't handle index shifting when restoring multiple inserted rows. The code processes rows in reverse order (ascending indices) which is correct for insertion, but it doesn't shift the indices of other inserted rows and changes that come after each restored row. When a row is restored, all subsequent inserted row indices should be incremented. Without this adjustment, the indices in insertedRowIndices and changes array will be incorrect, potentially causing issues with future operations.
